Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Resolve #390] Cast all parameters to strings #401

Closed
wants to merge 1 commit into from
Closed

[Resolve #390] Cast all parameters to strings #401

wants to merge 1 commit into from

Conversation

matrinox
Copy link
Contributor

Problem

Sceptre allows you to pass non-string values as parameters. Boto3 only allows string parameters. Refer to #390 for more details.

Example config:

parameters:
  Port: 3000
  RuleNumber: 500
  LoggingEnabled: True

Solution

To solve this, all non-string values are coerced into strings. An exception is made for booleans (True/False becomes "true"/"false"). List values are now recursively formatted. This has the side effect of supporting nested array values for parameters (no tests for this).

Alternatives

  1. Create boto3 PR to support non-string parameters for CloudFormation create/update stack APIs. This is also a possibility but it doesn't hurt to cast the values to strings here as well.
  2. Raise exception when passing non-string values. This is slightly better than current – more descriptive error messages – but personally, it isn't as good as the proposed solution. Development time is roughly the same.
  3. Do nothing or add this to readme. This is definitely the simplest and least riskiest solution but the UX is weaker than point Load only external hook classes required by relevant stack config(s) #2.

JuanCanham
JuanCanham reviewed Jun 14, 2018
View reviewed changes
sceptre/stack.py Outdated
# recurse
new_list = [self._format_value(item) for item in value]
return ",".join(new_list)
if value is True:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to downcase this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your parameter is defined like this:

Parameters:
  Key:
    Type: String
    AllowedValues:
      - False
      - True

Only "false"/"true" will work, not "False"/"True". Same goes for False/True. So you need to downcase it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

JuanCanham
JuanCanham reviewed Jun 28, 2018
View reviewed changes
Copy link

@JuanCanham JuanCanham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ngfgrant who is authorised to approve this it looks good to me though

@matrinox matrinox force-pushed the support-non-string-type-parameters branch from d6d017c to e1c1c77 Compare July 5, 2018 19:36
@matrinox
Copy link
Contributor Author

matrinox commented Aug 1, 2018

@ngfgrant Can you look into this PR please?

@matrinox matrinox force-pushed the support-non-string-type-parameters branch from e1c1c77 to 903b66f Compare August 1, 2018 21:02
@ngfgrant
Copy link
Contributor

ngfgrant commented Aug 2, 2018

Hey @matrinox - yeh this is definitely on my list. I am aiming to get a v1 release out this week and then will look at this as potentially something for v2 release. It is definitely on my list though ;)

@matrinox
Copy link
Contributor Author

matrinox commented Aug 2, 2018

Awesome, thanks for the update!

@ngfgrant
Copy link
Contributor

@matrinox I am going to get another couple of PRs in relating to refactoring the way config is handled and then after I am done would it be ok if I ask you to update this from master? There is not big changes, mainly that the methods on stack have moved from that object so your code just needs pasted elsewhere 😄

@matrinox
Copy link
Contributor Author

@ngfgrant When you finish doing that, please mention again and I will get to it :).

@ngfgrant ngfgrant force-pushed the master branch 2 times, most recently from 901d20a to 05c6742 Compare November 22, 2018 15:26
@ngfgrant ngfgrant changed the base branch from master to v1.5.0beta December 8, 2018 14:11
@ngfgrant ngfgrant changed the base branch from v1.5.0beta to v2.0.1 December 8, 2018 14:11
@ngfgrant ngfgrant changed the base branch from v2.0.1 to master December 17, 2018 15:12
@stig
Copy link
Contributor

stig commented Jul 11, 2019

Any chance of this being picked up? Numbers / booleans in the stack config file not being coerced to strings and this causing failure is the most frequent cause of git commit --fixup at our shop!

@matrinox matrinox force-pushed the support-non-string-type-parameters branch 2 times, most recently from 8bc6895 to bc830e2 Compare July 11, 2019 23:15
@matrinox
Cast all parameters to strings
971cd9a 
boto3 only supports strings for parameters, even if the parameter itself is non a string type
On branch support-non-string-type-parameters - Wed 13 Jun 2018 13:25:46 PDT by Geoff Lee <geofflee25@gmail.com>
@matrinox matrinox force-pushed the support-non-string-type-parameters branch from bc830e2 to 971cd9a Compare July 11, 2019 23:17
@alex-harvey-z3q
Copy link
Contributor

Any chance of this or similar being merged? This has been a big pain point for my team.

@matrinox
Copy link
Contributor Author

matrinox commented Jul 29, 2020 •
edited
Loading

@alexharv074 we already migrated to terraform so I'm not interested in updating this branch any longer. Sorry :(

You're welcome to take the code and run with it though, I don't mind.

@matrinox matrinox closed this Jul 29, 2020
@zaro0508
Copy link
Contributor

I have proposed an alternative idea to help with this in #925

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuanCanham JuanCanham JuanCanham left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@matrinox @ngfgrant @stig @alex-harvey-z3q @zaro0508 @JuanCanham